Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add contributor guide #5426

Merged
merged 5 commits into from
Nov 3, 2023

Conversation

ncapps
Copy link
Contributor

@ncapps ncapps commented Oct 30, 2023

Add a contributor guide.

Resolves #5199

@k8s-ci-robot
Copy link
Contributor

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 30, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @ncapps. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 30, 2023
@ncapps ncapps marked this pull request as ready for review October 30, 2023 21:44
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2023
@ncapps ncapps changed the title Add contribute guide Add contributor guide Oct 30, 2023
@annasong20
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 30, 2023
@ncapps
Copy link
Contributor Author

ncapps commented Oct 30, 2023

/label tide/merge-method-squas

@k8s-ci-robot
Copy link
Contributor

@ncapps: The label(s) /label tide/merge-method-squas cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label tide/merge-method-squas

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ncapps
Copy link
Contributor Author

ncapps commented Oct 30, 2023

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 30, 2023
@antoooks
Copy link
Contributor

Looks good to me

/lgtm

@k8s-ci-robot
Copy link
Contributor

@antoooks: changing LGTM is restricted to collaborators

In response to this:

Looks good to me

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Member

@bugoverdose bugoverdose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor comments.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated
Comment on lines 52 to 51
You will need to periodically fetch changes from the `upstream` repository to keep your working branch in sync.
```bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below is an example of creating a new branch to work on.

Maybe we can add example of syncing working branches that were already created on upstream repository changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. I think it'd be a good idea to split the part that speaks to syncing a fork from the part that creates a new working branch.

Copy link
Member

@charles-chenzz charles-chenzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the contributor side, this lgtm
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2023
Copy link
Member

@stormqueen1990 stormqueen1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ncapps, thanks for working on this! 😄

I left a few suggestions in comments and a small correction for a link that's not currently working.

CONTRIBUTING.md Outdated
Comment on lines 52 to 51
You will need to periodically fetch changes from the `upstream` repository to keep your working branch in sync.
```bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. I think it'd be a good idea to split the part that speaks to syncing a fork from the part that creates a new working branch.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 1, 2023
@ncapps ncapps force-pushed the docs/contributor-guide branch from 0a423e2 to be35c55 Compare November 1, 2023 18:05
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 1, 2023
Copy link
Member

@charles-chenzz charles-chenzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only one minor question

CONTRIBUTING.md Outdated Show resolved Hide resolved
@ncapps ncapps force-pushed the docs/contributor-guide branch from 8947d18 to 08fa96f Compare November 3, 2023 03:59
Copy link
Contributor

@natasha41575 natasha41575 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

/hold in case there are other changes you want to make before this merges

Feel free to remove the hold with /hold cancel when you are ready to merge (or ping me if the bot doesn't respond to your command

cd kustomize

# Configure upstream
git remote add upstream https://github.com/kubernetes-sigs/kustomize
Copy link
Contributor

@natasha41575 natasha41575 Nov 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Genuine question (you don't have to change anything if this is how you have yours set up, since it seems to be working great) - Does it make a difference if we use the ssh URL for the upstream vs the http one? I have my upstream configured with the ssh one, but I'm not sure if that actually affects anything in our workflows

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only difference I think I've observed is that

  • ssh: you need to authenticate for push and pull, but you can use 2 factor
  • http: you only need to authenticate for push, but you need to enter your password
    Given that we don't need to push to upstream, I think http is probably easier

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natasha41575, ncapps

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2023
@ncapps
Copy link
Contributor Author

ncapps commented Nov 3, 2023

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2023
@k8s-ci-robot k8s-ci-robot merged commit e7c8ed1 into kubernetes-sigs:master Nov 3, 2023
5 checks passed
antoooks pushed a commit to antoooks/kustomize that referenced this pull request Nov 21, 2023
* Add contributor guide

* git merge upstream/master

* git rebase upstream/master

* make test-unit-all

* make lint
@ncapps ncapps mentioned this pull request Dec 1, 2023
@ncapps ncapps deleted the docs/contributor-guide branch December 2, 2023 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
Development

Successfully merging this pull request may close these issues.

Create contributor guide
9 participants